Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

transport: remove RequireHandshakeHybrid support #2529

Merged
merged 1 commit into from
Feb 27, 2019
Merged

transport: remove RequireHandshakeHybrid support #2529

merged 1 commit into from
Feb 27, 2019

Conversation

jeanbza
Copy link
Member

@jeanbza jeanbza commented Dec 20, 2018

This removes RequireHandshakeHybrid support and changes the default behavior
to RequireHandshakeOn. Dial calls will now block and wait for a successful
handshake before proceeding. Users relying on the old hybrid behavior (cmux
users) should consult soheilhy/cmux#64.

Also, several tests have been updated to take this into consideration by
sending settings frames.

@jeanbza jeanbza requested a review from dfawley December 20, 2018 23:20
@jeanbza jeanbza force-pushed the remove_hybrid_mode branch 2 times, most recently from a546d75 to c5923c5 Compare December 21, 2018 00:07
@menghanl menghanl self-assigned this Jan 3, 2019
@dfawley dfawley assigned dfawley and unassigned menghanl Jan 10, 2019
dfawley
dfawley reviewed Jan 12, 2019
View reviewed changes
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but there might be room to delete even more!

clientconn.go Outdated
@@ -1229,6 +1163,22 @@ func (ac *addrConn) createTransport(addr resolver.Address, copts transport.Conne
return nil, err
}

if ac.dopts.reqHandshake == envconfig.RequireHandshakeOn {
select {
case <-prefaceTimer.C:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't still need a prefaceTimer, right? We should be able to just use the normal connect deadline here I think. And we can eliminate prefaceReceived, too, if we make NewClientTransport itself block until the preface is received. That should simplify things even more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • prefaceTimer: removed
  • onPrefaceReceipt: do you mind if I do that in a separate PR?

clientconn_test.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned jeanbza and unassigned dfawley Jan 12, 2019
@dfawley
Copy link
Member

dfawley commented Jan 14, 2019

@jadekler I think I'll send a minimal PR to disable this feature for the release tomorrow to reduce risk, and we can hold this PR until after. SG?

@jeanbza
Copy link
Member Author

jeanbza commented Jan 14, 2019

That SGTM.

@dfawley
Copy link
Member

dfawley commented Jan 14, 2019 •
edited
Loading

And per #2406 we aren't supposed to remove this support until the 1.19 release anyway, so I actually just re-stated the official plan.

@jeanbza jeanbza force-pushed the remove_hybrid_mode branch 2 times, most recently from 1e08f17 to a195f7c Compare February 8, 2019 22:10
@jeanbza jeanbza assigned dfawley and unassigned jeanbza Feb 8, 2019
@dfawley
Copy link
Member

dfawley commented Feb 11, 2019

There an unused import making travis fail; can you fix it please?

@jeanbza
Copy link
Member Author

jeanbza commented Feb 12, 2019

Shoot. Sorry. Done, and rebased.

@jeanbza jeanbza assigned jeanbza and unassigned dfawley Feb 20, 2019
@jeanbza
transport: remove RequireHandshakeHybrid support
4207789 
This removes RequireHandshakeHybrid support and changes the default behavior
to RequireHandshakeOn. Dial calls will now block and wait for a successful
handshake before proceeding. Users relying on the old hybrid behavior (cmux
users) should consult soheilhy/cmux#64.

Also, several tests have been updated to take this into consideration by
sending settings frames.
@jeanbza jeanbza assigned dfawley and unassigned jeanbza Feb 20, 2019
dfawley
dfawley approved these changes Feb 26, 2019
View reviewed changes
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but let's delay this until after today's release. We theoretically should remove the hybrid mode support in today's release, but going a little slower than planned to remove a feature is better than rushing a CL in right before a release.

@jeanbza
Copy link
Member Author

jeanbza commented Feb 26, 2019

SGTM.

@jeanbza jeanbza merged commit 5878d96 into grpc:master Feb 27, 2019
@dfawley dfawley added the Type: Behavior Change Behavior changes not categorized as bugs label Feb 28, 2019
@dfawley dfawley added this to the 1.20 Release milestone Feb 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dfawley dfawley dfawley approved these changes

Assignees

@dfawley dfawley

Labels
Type: Behavior Change Behavior changes not categorized as bugs
Projects
None yet
Milestone
1.20 Release
Development

Successfully merging this pull request may close these issues.
3 participants
@jeanbza @dfawley @menghanl